Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Improve nrnivmodl-core workflow and integration with nmodl/mod2c #388

Merged
merged 35 commits into from
Sep 21, 2020
Merged

Improve nrnivmodl-core workflow and integration with nmodl/mod2c #388

merged 35 commits into from
Sep 21, 2020

Conversation

pramodk
Copy link
Collaborator

@pramodk pramodk commented Aug 31, 2020 •
edited
Loading

  • Main motivation of this PR is to streamline workflow so that the MOD files can be compiled for different backends or using different MOD2C/NRNIVMODL flags.
  • Currently inbuilt mechanisms are compiled during CoreNEURON build time. If user want to compile mod files with different NMODL flags, it's not possible to do it without re-compiling CoreNEURON itself.
  • In this PR we removed compilation of mod files from CMake as part of libcoreneuron.
  • Mod files are now compiled with Makefile + nrnivmodl-core script and resultant library is linked to special-core or nrniv-core.
  • Build all mod files uniformly by removing NmodlHelper.cmake
    everyone now use nrnivmodl-core to build binary
  • internal mod files are now also compiled via nrnivmdl-core
  • ispc target also use nrnivmodl-core workflow
  • Remove extra library libcorenrnmech built
  • Remove dest option in nrnivmld-core
  • Remove nrnivmodl_options.txt SAVE file
  • Avoid installation of libcorenrnmech into bin directory
  • Cleanup and simplify nrnivmodl-core.in and nrnivmodl_core_makefile.in

@pramodk pramodk force-pushed the pramodk/nrnivmodl-uniform-workflow branch from 65fd5bd to bdd1aaf Compare September 4, 2020 08:45
@pramodk pramodk requested review from ferdonline, ohm314, iomaganaris and alexsavulescu and removed request for iomaganaris September 7, 2020 21:53
@pramodk
Copy link
Collaborator Author

pramodk commented Sep 7, 2020

@ferdonline @alexsavulescu @ohm314 @iomaganaris : I don't know Makefile well but I have significantly changed implementation to make scripts & makefie bit easy to understand. Take a quick look and we can see what needs to be changed.

ferdonline
ferdonline reviewed Sep 8, 2020
View reviewed changes
extra/nrnivmodl-core.in

# copy mod files with include files. note that ${ROOT_DIR}/share
Copy link
Contributor

@ferdonline ferdonline Sep 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is we prepare the mod files here so that the makefile can be simpler?

Btw, for reference, maybe we can update https://user-images.githubusercontent.com/915100/53173802-ad701780-35e8-11e9-9d28-0441aecbf517.png (in #133 (comment))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferdonline : main motivation for recompiling all mod files is to have flexibility in building special. For example, once CoreNeuron is installed, we want to compile mod files with mod2c or nmodl or different nmodl options or different compiler flags.

If inbuilt mod files are built inside libcoreneuron, one has to completely re-install CoreNeuron with different flags of nmodl. This is quite restrictive for new usage with NMODL. And hence, we no longer want to compile mod files inside libcoreneuron.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, for reference, maybe we can update https://user-images.githubusercontent.com/915100/53173802-ad701780-35e8-11e9-9d28-0441aecbf517.png (in #133 (comment))

Could you provide the source of that? :)

@ferdonline
Copy link
Contributor

Really cool to have dropped this old mode of building mods in the main Cmake and removed NmodlHelper.cmake.
As an idea maybe we can move some logic from the makefile to the bash script. bash is not pretty (maybe we could think about Python ?) but certainly makefile functions are harder to maintain. I believe keeping the makefile to the bare role of building while taking care of dependencies would be good.

ohm314
ohm314 reviewed Sep 11, 2020
View reviewed changes
Copy link
Contributor

@ohm314 ohm314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice. Love the cleanup of the makefile! I just have a few questions/minor comments.

iomaganaris
iomaganaris reviewed Sep 15, 2020
View reviewed changes
Copy link
Contributor

@iomaganaris iomaganaris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. I don't have much to add on the comments of the others

@pramodk
Copy link
Collaborator Author

pramodk commented Sep 15, 2020

As an idea maybe we can move some logic from the makefile to the bash script. bash is not pretty (maybe we could think about Python ?) but certainly makefile functions are harder to maintain.

@ferdonline : I agree that this could be further simplified and cleaned. I am thinking that we should make such change in separate PR so that we don't change too many things in the same PR (especially introducing new things with bash or python). With the current changes, I am more or less confident that it won't break too many things :)

@pramodk
Copy link
Collaborator Author

pramodk commented Sep 16, 2020

Please retest

@pramodk
WIP : make now builds special-core for tests
3237bd4
@pramodk
nrnivmodl-core cleanup
20c3714 
 - remove destination option
 - avoid re-compilation of mod files
 - minor cleanup of makefile - remove unused variables and install target
@pramodk
Remove unused CMake module and code for new workflow
f9fda3c
@pramodk
- remove libnrnmech which is not used (?)
d0b6e8c 
- install special-core as nrniv-core
- install libcorenrnmech into bin directory
- rmeove unused cmake code
@pramodk
apply cmake-format
a1afb6a
@pramodk
install mod files from /share
14bdb07
@pramodk
fix test links
add49a7
@pramodk
Add dependency between coreneuron_test for mech library
ecaebff
@pramodk
linking fixes for GPU build
bc50840
@pramodk
Small changes/refactoring in nrnivmodl-core.in
fbcb09f
@pramodk
intermediate commit
d73a646
@pramodk
Refactoring of target rules
7637e14
@pramodk
Cleanup makefile rules more
23cd2df
@pramodk
refactoring more targets
abb1b0b
@pramodk
more makefile refactor
2277111
@pramodk
fixed non-ispc targets
eae303d
@pramodk
fix missing single quotes for rpath
a6540f5
@pramodk
symlink nrnunits
6ed9a26
@pramodk
travis verbose
f3b7081
@pramodk
fix link error in static builds
1fc9b8e
@pramodk
Set visiblity to hidden to avoid double free
d82a8c8
@pramodk
undo visibility hidden; instead avoid linking licoreneuron
0ac5343
@pramodk
Set rpath for build time exes
6763bd3
@pramodk
Add coreneuron library linking
d0b9961
@pramodk
libcoreneuron is built with SHARED and vice-a-versa
2678cc2
@pramodk
Allow undefined reference in shared library
1ced129
@pramodk
limit to two parallel builds with make
5208532
@pramodk
Add dependency with nmodl target
e461e0c
@pramodk
More cleanup and documentation; serial build
ab3c381
@pramodk
rename target used by neuron : coreneuron_test to nrniv-core
c8501c9
@pramodk
update tests with the target
a4d36dc
@pramodk
Change INSTALL_ to CORENRN_ prefix and address review comment
250eb92
@pramodk
Add install target and re-introduce -d option by popular demand
8dfa544
@pramodk pramodk force-pushed the pramodk/nrnivmodl-uniform-workflow branch from 1c87231 to 8dfa544 Compare September 21, 2020 11:27
@pramodk pramodk marked this pull request as ready for review September 21, 2020 11:28
@pramodk
Copy link
Collaborator Author

pramodk commented Sep 21, 2020

Please retest

ohm314
ohm314 approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@ohm314 ohm314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@pramodk pramodk merged commit cc85723 into master Sep 21, 2020
@pramodk pramodk deleted the pramodk/nrnivmodl-uniform-workflow branch September 21, 2020 17:05
pramodk added a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022
@pramodk
Improve nrnivmodl-core workflow and integration with nmodl/mod2c (Blu…
1f8bb6e 
…eBrain/CoreNeuron#388)

* nrnivmodl-core cleanup
  - remove destination option
  - avoid re-compilation of mod files
  - minor cleanup of makefile - remove unused variables and install target
* Remove unused CMake module and code for new workflow
  - remove libnrnmech which is not used (?)
  - install special-core as nrniv-core
  - install libcorenrnmech into bin directory
  - rmeove unused cmake code
* install mod files from /share
* fix test links
* Add dependency between coreneuron_test for mech library
* linking fixes for GPU build
* Small changes/refactoring in nrnivmodl-core.in
* Refactoring of target rules
* Cleanup makefile rules more
* Add dependency with nmodl target
* Change INSTALL_ to CORENRN_ prefix and address review comment
* Build libcornrnmech static one always required for nrniv-core
* Add CLI option to choose static or shared build

CoreNEURON Repo SHA: BlueBrain/CoreNeuron@cc85723
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ferdonline ferdonline ferdonline left review comments

@ohm314 ohm314 ohm314 approved these changes

@iomaganaris iomaganaris iomaganaris approved these changes

@alexsavulescu alexsavulescu Awaiting requested review from alexsavulescu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pramodk @ferdonline @ohm314 @iomaganaris